🐛 fix(taint deletion): allow taint changes and prevent panic by handling nil values #5022

nueavv · 2024-06-13T08:59:09Z

This commit addresses a panic error that occurs when attempting to delete a taint in AWSManagedMachinePool. The issue was caused by a nil pointer dereference when the taint's value was nil. By handling nil values for taint fields, this fix ensures that taints with nil values are correctly processed, preventing runtime errors.

Fixes #5021

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR resolves a panic error that occurs when attempting to delete a taint in AWSManagedMachinePool. The error was due to a nil pointer dereference when the taint's value was nil. By handling nil values for taint fields, this fix ensures that taints with nil values are correctly processed, preventing runtime errors.

Which issue(s) this PR fixes:
Fixes #5021

Special notes for your reviewer:

Checklist:

Release note:

Fixed a panic error during taint deletion in AWSManagedMachinePool by handling nil values for taint fields.

k8s-ci-robot · 2024-06-13T08:59:18Z

Hi @nueavv. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

richardcase · 2024-07-01T13:17:52Z

/ok-to-test

pkg/cloud/converters/eks.go

richardcase · 2024-07-17T10:34:30Z

Thanks for the this @nueavv . When you have time could you respond to @fiunchinho suggestion? Then i think we can look to get this merged.

nueavv · 2024-07-18T15:22:53Z

Okay. @richardcase

richardcase · 2024-07-18T15:55:44Z

@nueavv - there's a couple of failures with the checks. If you run the following locally (which is what he checks are running):

make lint
make test

Ping me on slack if i can help with anything.

nueavv · 2024-07-18T22:32:32Z

@richardcase I have fixed the issue. Can you please check if the commit message is appropriate?

richardcase · 2024-07-20T07:53:48Z

/test ?

k8s-ci-robot · 2024-07-20T07:53:50Z

@richardcase: The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-aws-build
/test pull-cluster-api-provider-aws-build-docker
/test pull-cluster-api-provider-aws-test
/test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass
/test pull-cluster-api-provider-aws-e2e-conformance
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e-eks-gc
/test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main
pull-cluster-api-provider-aws-build
pull-cluster-api-provider-aws-build-docker
pull-cluster-api-provider-aws-test
pull-cluster-api-provider-aws-verify

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

richardcase · 2024-07-20T07:54:00Z

/test pull-cluster-api-provider-aws-e2e-eks

AndiDog

Regarding the issue and according release note: I don't quite get whether the issue is about deletion (the taint shouldn't even exist in the slice?!) or an empty value string – let's please clarify. The GitHub issue says it's about an empty string, and it looks like this is not a deletion issue?!

nueavv · 2024-07-29T08:24:27Z

Hi @AndiDog,

I was trying to delete the taint, but it failed because of the empty string. What made you confused? The issue is about taint deletion.

nueavv · 2024-07-29T08:26:29Z

You can reproduce it by first creating a taint without a value and then trying to delete it.

nueavv · 2024-08-10T07:22:50Z

@dlipovetsky Hi! can you review this?

AndiDog · 2024-08-29T07:13:49Z

The change now looks straightforward. Can you please make this a single commit? I'll take a look very soon – it's on my list 😉.

pkg/cloud/converters/eks_test.go

pkg/cloud/converters/eks.go

k8s-triage-robot · 2025-02-07T05:21:04Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Ankitasw · 2025-02-10T09:17:05Z

/lgtm

AndiDog · 2025-02-12T18:45:55Z

pkg/cloud/converters/eks_test.go

+func awsString(value string) *string {
+	return &value
+}


We already use ptr.To[string](...) for this (or aws.String(...) in older code)

(minor, rest looks good)

richardcase · 2025-03-04T17:26:03Z

This has been open a very long time, sorry @nueavv.

Any chance you could rebase and we can get this in? Also, might be worth making the change @AndiDog mentions.

nueavv · 2025-03-05T00:17:36Z

@richardcase I rebased it and fixed it. does it look good ?

richardcase · 2025-03-05T16:15:28Z

/retest

richardcase · 2025-03-06T08:32:26Z

/test pull-cluster-api-provider-aws-test

richardcase · 2025-03-06T10:07:53Z

/lgtm
/approve

k8s-ci-robot · 2025-03-06T10:08:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndiDog, richardcase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AndiDog,richardcase]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-triage-robot · 2025-04-05T11:06:20Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2025-05-05T11:48:52Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2025-05-05T11:48:57Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot requested review from AndiDog and dlipovetsky June 13, 2024 08:59

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 13, 2024

nueavv changed the title ~~🐛 fix(taint deletion): handle nil values to prevent panic~~ 🐛 fix(taint deletion): allow taint changes and prevent panic by handling nil values Jun 14, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 1, 2024

fiunchinho reviewed Jul 15, 2024

View reviewed changes

pkg/cloud/converters/eks.go Outdated Show resolved Hide resolved

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 17, 2024

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 18, 2024

AndiDog reviewed Jul 29, 2024

View reviewed changes

richardcase reviewed Nov 4, 2024

View reviewed changes

pkg/cloud/converters/eks_test.go Outdated Show resolved Hide resolved

pkg/cloud/converters/eks.go Outdated Show resolved Hide resolved

nueavv force-pushed the fix/taints branch from ff3fc1d to 6c3cc71 Compare November 9, 2024 05:14

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 7, 2025

k8s-ci-robot assigned Ankitasw Feb 10, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2025

AndiDog reviewed Feb 12, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2025

nueavv force-pushed the fix/taints branch from 6c3cc71 to 7c65006 Compare March 5, 2025 00:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2025

nueavv force-pushed the fix/taints branch 2 times, most recently from 7c65006 to 0dfd87d Compare March 6, 2025 07:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2025

fix(taint deletion): handle nil values to prevent panic

e2318bc

nueavv force-pushed the fix/taints branch from 0dfd87d to e2318bc Compare March 6, 2025 07:24

k8s-ci-robot assigned richardcase Mar 6, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2025

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 5, 2025

k8s-ci-robot closed this May 5, 2025

🐛 fix(taint deletion): allow taint changes and prevent panic by handling nil values #5022

🐛 fix(taint deletion): allow taint changes and prevent panic by handling nil values #5022

Uh oh!

Conversation

nueavv commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 13, 2024

Uh oh!

richardcase commented Jul 1, 2024

Uh oh!

Uh oh!

richardcase commented Jul 17, 2024

Uh oh!

nueavv commented Jul 18, 2024

Uh oh!

richardcase commented Jul 18, 2024

Uh oh!

nueavv commented Jul 18, 2024

Uh oh!

richardcase commented Jul 20, 2024

Uh oh!

k8s-ci-robot commented Jul 20, 2024

Uh oh!

richardcase commented Jul 20, 2024

Uh oh!

AndiDog left a comment

Choose a reason for hiding this comment

Uh oh!

nueavv commented Jul 29, 2024

Uh oh!

nueavv commented Jul 29, 2024

Uh oh!

nueavv commented Aug 10, 2024

Uh oh!

AndiDog commented Aug 29, 2024

Uh oh!

Uh oh!

Uh oh!

k8s-triage-robot commented Feb 7, 2025

Uh oh!

Ankitasw commented Feb 10, 2025

Uh oh!

AndiDog Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

richardcase commented Mar 4, 2025

Uh oh!

nueavv commented Mar 5, 2025

Uh oh!

richardcase commented Mar 5, 2025

Uh oh!

richardcase commented Mar 6, 2025

Uh oh!

richardcase commented Mar 6, 2025

Uh oh!

k8s-ci-robot commented Mar 6, 2025

Uh oh!

k8s-triage-robot commented Apr 5, 2025

Uh oh!

k8s-triage-robot commented May 5, 2025

Uh oh!

k8s-ci-robot commented May 5, 2025

Uh oh!

Uh oh!

nueavv commented Jun 13, 2024 •

edited

Loading